-
Notifications
You must be signed in to change notification settings - Fork 15
wip #200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
wip #200
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run //src:license-check Status: Click to expand output
|
The created documentation from the pull request is available at: docu-html |
|
||
The subset is explicit rather than dynamically inferred. Tests in it should fail quickly | ||
when contracts or shared schemas drift. If the list grows until it is slow it will | ||
either be disabled or ignored; regular curation keeps it useful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disabling/ignoring should be impossible for anyone who is not an administrator. The passing of the subset must be a pre-requisite to even "enabling the merge button".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was thinking of empowered developers. The other alternative would simply be extreme inefficiency in development. Not better, just a different hell.
### Coordinated Multi-Repository Subset | ||
Some changes require multiple repositories to move together (for example a schema | ||
evolution, a cross-cutting refactor, a protocol tightening). We mark related pull | ||
requests using a stable mechanism such as a common label (e.g. changeset:feature-x). The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about simply using the same HEAD branch name to identify a changeset? We had reasonable experience with that. If combined with draft PRs, it is even possible to automatically create "downstream" PRs (e.g. in the integration repository) since in that case also the intended BASE branch is known.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sure. Labels are just an example here. Alignment of branch names might be more complex across different modules and different companies. And forks.
back to each pull request. None merge until the coordinated set is green. This removes | ||
informal merge ordering as a coordination mechanism. | ||
|
||
### Post-Merge Full Suite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO it would be very nice if that could be avoided, but maybe it cannot - sure. However, I would introduce it only when the splitting of pre- and post-merge is really required, i.e. when the "WTFs/min" mentioned by developers are getting too high ;-)
One more remark: it may be that merge queues can help. We have seen that it can work very well to have "cheap, often failing" tests run on PR push, and run "expensive, seldomly failing" tests in the merge queue. That way, people can click merge, go home, expect that they are "usually done". Merge queue does "the rest". Only in rare cases where the cheap tests have missed something, the queue will fail. Well, OK - fine. Fix it, write a cheap regression test so that it does not happen again - move on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the scope of the project, we'll not manage to keep tests fast enough to run all of them pre-merge. There will be slow system image generation or HW tests.
GitHub merge queues I have not included in the concept. Let's investigate how exactly they fit in. AFAIK they are pre-merge as well and simply combine multiple PRs?
|
||
Large configuration belongs elsewhere; manifests should stay readable and diffable. | ||
|
||
## Example: GitHub Actions (Conceptual) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting example, thanks for that!
SHAs if multiple components advanced). On success we append a record for the tuple with | ||
a manifest hash and timing data. | ||
|
||
### Manifests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a huge Bazel fan but - in S-CORE we should be able to use Bazel lockfiles here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we can/should mess with lock files. I was thinking of adding git_override statements like this
docs-as-code/src/tests/test_consumer.py
Lines 187 to 199 in 6970b40
def replace_bazel_dep_with_git_override( | |
module_content: str, git_hash: str, gh_url: str | |
) -> str: | |
pattern = r'bazel_dep\(name = "score_docs_as_code", version = "[^"]+"\)' | |
replacement = f'''bazel_dep(name = "score_docs_as_code", version = "0.0.0") | |
git_override( | |
module_name = "score_docs_as_code", | |
remote = "{gh_url}", | |
commit = "{git_hash}" | |
)''' | |
return re.sub(pattern, replacement, module_content) |
unrelated stuff: